Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature/add-a-health-endpoint #35

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bartekus
Copy link

Resolve Add a /health endpoint #5

Signed-off-by: Bartlomiej P Kus [email protected]

@bartekus
Copy link
Author

That should do it, sorry for the overhead, but I had to recreate PR @swcurran

@swcurran swcurran requested a review from WadeBarnes June 28, 2022 14:31
@WadeBarnes
Copy link
Member

@bartekus, Thanks for the contribution! The only thing that is missing from the issue request is the "meaningful health check on the server's services". As it stands the /health/check endpoint will return success if the web server is running, but that doesn't provide any indication of whether the tail-server is in a working condition.

That said, a health check is better than no health check. I'd be fine to accept and merge, we'd just need to create a new issue to track the addition of the check on the server's services.

Would you like to take a stab at the check, or defer that for another time?

@bartekus
Copy link
Author

That's a good idea, let me have a stab at it, no point in merging half-baked implementations!

bartekus added 2 commits June 28, 2022 16:31
Signed-off-by: Bartlomiej P Kus <[email protected]>
Signed-off-by: bartekus <[email protected]>
Signed-off-by: Bartlomiej P Kus <[email protected]>
Signed-off-by: bartekus <[email protected]>
@bartekus bartekus force-pushed the feature/add-a-health-endpoint branch from 7aae33b to 7b8daa6 Compare June 28, 2022 22:31
@bartekus
Copy link
Author

I've expanded on the solution to enable extensibility as well as add second endpoint to facilitate setting/stats dump.
I've take the cues from other implementations (nodejs/flask) but python is not my speciality but I did what I could with the help of PhCharm CE.

@WadeBarnes
Copy link
Member

Thanks @bartekus. I'll test it out.

@Executioner1939
Copy link

Hi @WadeBarnes is there anyway this can be merged? Anything I can do to help? I quite need an HTTP healthcheck in my environment

@WadeBarnes
Copy link
Member

I've had a quick look. Overall the updates look fine. Care would need to be taken to ensure the /health/env endpoint was never exposed publicly as it provides too much information about the host system, though I could see it coming in handy for some troubleshooting purposes.

@esune, @loneil, Your thoughts?

@WadeBarnes
Copy link
Member

There are some minor conflicts that I can clean up easily to get things up to date with the latest commits.

@esune
Copy link
Member

esune commented Apr 26, 2024

@esune, @loneil, Your thoughts?

For other projects (i.e.: ACA-Py) we protect the sensitive endpoints with the requirement for an api-key or other authentication method. Maybe that's what we should/could do here?

@esune
Copy link
Member

esune commented Apr 26, 2024

@bartekus wondering what the purpose of the env endpoint is, since it discloses information about the host and stack (and to @WadeBarnes point, it might need to be protected if we keep it)

@Executioner1939
Copy link

Thanks for looking into this, for now we simply forked the repo and added a basic health check endpoint that just returns a 200 OK as we were in a deadline pinch, however the "fix" is not a proper health / readiness check.

We need this as GKE's Ingress Controller set's up an HTTP(S) Global Loadbalancer which requires that all NEGs pass a GoogleHC before it routes traffic and it can't be disabled.

@bartekus
Copy link
Author

You are right, this isn't very good approach, alas I did not revisit this in quite a while, I've rewritten the whole thing in Rust (as learning activity (but never share with anyone)) so perhaps if there's an appetite and further specification re: requirements for the health / readiness check I can implement it and share with the OSS at large

Alternatively I can amend the existing python version, altho its not a language of my passion as rust FYI

@WadeBarnes
Copy link
Member

@bartekus, While a rust implementation is intriguing, I think it would be better handled as a completely separate topic. For the scope and context of this PR, removing the EnvDump implementation and associated API would be sufficient to wrap things up.

Let me know if you have the time and desire to handle that, otherwise we can arrange to make the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants